Stop unnecessarily scanning all files to report app configs by nakabonne · Pull Request #2854 · pipe-cd/pipecd

nakabonne · 2021-11-26T09:09:16Z

What this PR does / why we need it:
This PR mainly improves appconfigreporter to:

simplify the way to find out registered apps by using the cached app list.
solve the problem of scanning all files under the repository every time to look for unregistered applications, even though no new commits have been created.

For more information about the first one, please visit #2838.
The second one is solved by caching the latest unregistered apps.

Which issue(s) this PR fixes:

Fixes #2838
Close #2837

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2021-11-26T09:09:18Z

Unabled to get changed files of pull request.

Details

Error: rpc error: code = NotFound desc = not found

pipecd-bot · 2021-11-26T09:09:18Z

Failed while validating Kapetanios configuration.

Details

Error: rpc error: code = NotFound desc = not found

nakabonne · 2021-11-26T09:09:33Z

/kapetanios reload

pipecd-bot · 2021-11-26T09:09:36Z

@nakabonne: Successfully loaded the latest Kapetanios configuration.

pipecd-bot · 2021-11-26T09:16:17Z

Code coverage for golang is 31.63%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/piped/appconfigreporter/appconfigreporter.go	isSynced	`--`	`70.00%`	`+70.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateUnregisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findRegisteredApps	`83.33%`	`100.00%`	`+16.67%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findUnregisteredApps	`91.30%`	`78.57%`	`-12.73%`
pkg/config/config.go	Config.init	`88.00%`	`92.00%`	`+4.00%`
pkg/config/config.go	Config.UnmarshalJSON	`91.67%`	`100.00%`	`+8.33%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.scanAppConfigs	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateRegisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	makeGitPathKey	`100.00%`	`--`	`-100.00%`

nghialv

Looks neat! Good job!
Just some nits.

pkg/app/piped/appconfigreporter/appconfigreporter.go

nghialv · 2021-11-29T02:40:14Z

pkg/app/piped/appconfigreporter/appconfigreporter.go

+	if appInfo.Kind != app.Kind {
+		return false
+	}


Changing the Kind could cause unexpected behavior since the application is paired with a Cloud Provider selected while adding the app.
So I think that field should not be allowed to be changed. How do you think?

Actually I feel the same way. Currently changing it via the web client is allowed, but few users use it possibly.

Applied at f05df12

Co-authored-by: Le Van Nghia <[email protected]>

pipecd-bot · 2021-11-29T02:57:01Z

Code coverage for golang is 31.63%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/piped/appconfigreporter/appconfigreporter.go	isSynced	`--`	`70.00%`	`+70.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateUnregisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findRegisteredApps	`83.33%`	`100.00%`	`+16.67%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findUnregisteredApps	`91.30%`	`78.57%`	`-12.73%`
pkg/config/config.go	Config.init	`88.00%`	`92.00%`	`+4.00%`
pkg/config/config.go	Config.UnmarshalJSON	`91.67%`	`100.00%`	`+8.33%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.scanAppConfigs	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateRegisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	makeGitPathKey	`100.00%`	`--`	`-100.00%`

pipecd-bot · 2021-11-29T03:04:47Z

Code coverage for golang is 31.63%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/piped/appconfigreporter/appconfigreporter.go	isSynced	`--`	`75.00%`	`+75.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.scanAppConfigs	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateRegisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateUnregisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findRegisteredApps	`83.33%`	`100.00%`	`+16.67%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findUnregisteredApps	`91.30%`	`78.57%`	`-12.73%`
pkg/config/config.go	Config.init	`88.00%`	`92.00%`	`+4.00%`
pkg/config/config.go	Config.UnmarshalJSON	`91.67%`	`100.00%`	`+8.33%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	makeGitPathKey	`100.00%`	`--`	`-100.00%`

nakabonne · 2021-11-29T03:06:23Z

I changed to emit log when kind has been changed

nghialv · 2021-11-29T03:12:28Z

Thank you. So let's remove this too.
https://github.com/pipe-cd/pipe/blob/bb8483359a18c71736a24dac80c5fcbeba154d79/pkg/app/api/grpcapi/piped_api.go#L983

pipecd-bot · 2021-11-29T03:13:31Z

Code coverage for golang is 31.63%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.isSynced	`--`	`70.00%`	`+70.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateRegisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateUnregisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findRegisteredApps	`83.33%`	`100.00%`	`+16.67%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findUnregisteredApps	`91.30%`	`78.57%`	`-12.73%`
pkg/config/config.go	Config.init	`88.00%`	`92.00%`	`+4.00%`
pkg/config/config.go	Config.UnmarshalJSON	`91.67%`	`100.00%`	`+8.33%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.scanAppConfigs	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	makeGitPathKey	`100.00%`	`--`	`-100.00%`

pipecd-bot · 2021-11-29T03:14:17Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Make it possible to follow the ApplicationInfo field changes

https://github.com/pipe-cd/pipe/blob/1acc33ea60aa55e6e99f7da0dcf820238872a11e/pkg/app/piped/appconfigreporter/appconfigreporter.go#L277-L280

This was created by todo plugin since "TODO:" was found in `1acc33e` when #2854 was merged. cc: @nakabonne.

nakabonne · 2021-11-29T03:14:27Z

@nghialv Fixed 👍

nghialv · 2021-11-29T03:21:04Z

Nice. Well done.
/lgtm

pipecd-bot · 2021-11-29T03:21:16Z

Code coverage for golang is 31.63%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.isSynced	`--`	`70.00%`	`+70.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findRegisteredApps	`83.33%`	`100.00%`	`+16.67%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.findUnregisteredApps	`91.30%`	`78.57%`	`-12.73%`
pkg/config/config.go	Config.init	`88.00%`	`92.00%`	`+4.00%`
pkg/config/config.go	Config.UnmarshalJSON	`91.67%`	`100.00%`	`+8.33%`
pkg/app/api/grpcapi/piped_api.go	PipedAPI.UpdateApplicationConfigurations	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.scanAppConfigs	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateRegisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	Reporter.updateUnregisteredApps	`0.00%`	`0.00%`	`+0.00%`
pkg/app/piped/appconfigreporter/appconfigreporter.go	makeGitPathKey	`100.00%`	`--`	`-100.00%`

khanhtc1202 · 2021-11-29T03:46:53Z

Nice work
/approve

pipecd-bot · 2021-11-29T03:46:55Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Stop unnecessarily scanning all files to report app configs

b97dcf2

pipecd-bot added the release-note-none label Nov 26, 2021

nghialv reviewed Nov 29, 2021

View reviewed changes

Apply suggestions from code review

33fc106

Co-authored-by: Le Van Nghia <[email protected]>

pipecd-bot added area/api area/piped size/L labels Nov 29, 2021

Disable Kind to be changed

f05df12

Emit log if kind has been changed

bb84833

Don't update kind

1acc33e

pipecd-bot added the lgtm label Nov 29, 2021

pipecd-bot added the approved label Nov 29, 2021

pipecd-bot merged commit 295adc5 into master Nov 29, 2021

pipecd-bot deleted the refactor-appconfigreporter branch November 29, 2021 03:55

This was referenced Nov 29, 2021

Make it possible to follow the ApplicationInfo field changes #2859

Closed

Update doc for deployment triggering #2867

Merged

Temporarily disable appconfig reporter in Piped #2868

Merged

Conversation

nakabonne commented Nov 26, 2021

Uh oh!

pipecd-bot commented Nov 26, 2021

Uh oh!

pipecd-bot commented Nov 26, 2021

Uh oh!

nakabonne commented Nov 26, 2021

Uh oh!

pipecd-bot commented Nov 26, 2021

Uh oh!

pipecd-bot commented Nov 26, 2021

Uh oh!

nghialv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nghialv Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

nakabonne Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

nakabonne Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Nov 29, 2021

Uh oh!

pipecd-bot commented Nov 29, 2021

Uh oh!

nakabonne commented Nov 29, 2021

Uh oh!

nghialv commented Nov 29, 2021

Uh oh!

pipecd-bot commented Nov 29, 2021

Uh oh!

pipecd-bot commented Nov 29, 2021

1. Make it possible to follow the ApplicationInfo field changes

This was created by todo plugin since "TODO:" was found in 1acc33e when #2854 was merged. cc: @nakabonne.

Uh oh!

nakabonne commented Nov 29, 2021

Uh oh!

nghialv commented Nov 29, 2021

Uh oh!

pipecd-bot commented Nov 29, 2021

Uh oh!

khanhtc1202 commented Nov 29, 2021

Uh oh!

pipecd-bot commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

This was created by todo plugin since "TODO:" was found in `1acc33e` when #2854 was merged. cc: @nakabonne.